-
-
Notifications
You must be signed in to change notification settings - Fork 442
Added gRPC functions to manage libraries in profiles #3019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3019 +/- ##
==========================================
- Coverage 69.42% 69.17% -0.26%
==========================================
Files 242 247 +5
Lines 18632 18935 +303
==========================================
+ Hits 12936 13099 +163
- Misses 4502 4604 +102
- Partials 1194 1232 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
As stated by @per1234: > Build profile data is already provided via the LoadSketch method, so it > seems that even a mechanism that is truly for getting profile data should > be implemented by simply expanding the SketchProfile message to contain > all the data of the build profile (actually kind of silly that it > currently only provides a subset of the profile data). arduino#3019 (comment)
| // Name of the library. | ||
| string name = 1; | ||
| // Version of the library if taken from the Library Index. | ||
| string version = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clients would love to use a dedicated message to add the latest version of the library (without listing the available versions), and maybe add the currently installed version that errors if the library is not installed.
dbac430 to
2327b35
Compare
|
Hello Cristian, Thanks for your great work 💪 I’ve been testing this area from the IDE side and was wondering how this feature is expected to work with an IDE or editor. Maybe this isn’t the most appropriate channel, but I guess you’re the best person to ask. It’s clear there’s a real need for this feature:
While reading the PR and the related IDE issue arduino/arduino-ide#2573, I was trying to understand how profiles are meant to behave over gRPC, maybe later in Arduino IDE 2.x:
I really like how profiles make builds reproducible and easy to share — that’s perfect for CI or collaborative work. But in an IDE that uses gRPC interactively, managing multiple isolated clients could get heavy, especially if it means repeated downloads. In my current setup I treat I’d be curious to hear how Arduino sees this working long-term on the gRPC side — especially for IDE integration. Will profiles act more like isolated environments or just as stored build settings that can be written and updated through gRPC? No hurry with this. Thanks a lot for your time |
|
Hi @dankeboy36! Thank you for your valuable feedback, as always.
The basic idea is that when a profile is in use, the platforms and libraries installed globally should have no effect, and only the platforms and libraries listed in the profile should be visible. This idea was implemented by adding the message InitRequest {
// An Arduino Core instance.
Instance instance = 1;
// Profile to use.
string profile = 2;
// The path where the sketch is stored.
string sketch_path = 3;
}An instance initialized with a profile should be able to see only the libraries listed in the profile, and I've actually checked that this is the case, using the following profile: If I try to initialize an instance of the CLI with the profile and try to get the list of installed libraries: err := srv.Init(&rpc.InitRequest{
Instance: inst,
SketchPath: sketchPath.String(),
Profile: "uno",
}, stream)
res, err := srv.LibraryList(ctx, &rpc.LibraryListRequest{
Instance: inst,
All: true,
})
require.NoError(t, err)
for _, lib := range res.InstalledLibraries {
fmt.Println("LIB>",
lib.GetLibrary().GetName(),
lib.GetLibrary().GetVersion(),
lib.GetLibrary().GetLocation(),
lib.GetLibrary().GetInstallDir())
}I got the expected: The Servo library is listed as So, based on the previous test, the answer to the following question is:
Yes, the result will differ depending on the profile. By the way, there is a distinction between a library search (i.e., searching for a library in the Library Index) and listing installed libraries. In the first case, searching for a library will always produce the same result regardless of the selected profile; however, listing the installed libraries will show different results based on the selected profile.
Libraries and platforms installed through profiles are placed in a private folder inside
The
The above must surely be fixed in the next releases. I'll open an issue for that. |
|
We are currently using this API in the AppLab IDE to manage profiles in the sketch. This is still experimental, and your suggestions and thoughts are more than welcome. Thanks for taking the time to write them down. 🙏🏼
At the moment, I would say that they serve both purposes:
The hard question is how all of this integrates with the IDE? My envisioned gRPC API involves the IDE creating multiple instances (using the Currently, I understand the Arduino IDE 2 uses a global instance (without a profile) that might be shared across all sketches that do not use profiles. I see that this could complicate things when the profiles are introduced because each UX business logic needs to be implemented twice, once for the classic "non-profiled" sketches and once for "profiled" sketches. There are still changes to the API to be made to make it IDE-friendly. The current state of the gRPC API is still not ready for that, but the above is the direction I'd like to see in the future. |
7c9f505 to
ea0038f
Compare
|
I've removed the Git portion from this PR #3028 because it needs more work. It will be merged and discussed separately. |
As stated by @per1234: > Build profile data is already provided via the LoadSketch method, so it > seems that even a mechanism that is truly for getting profile data should > be implemented by simply expanding the SketchProfile message to contain > all the data of the build profile (actually kind of silly that it > currently only provides a subset of the profile data). arduino#3019 (comment)
And also corresponding messages: InitProfileRequest -> ProfileCreateRequest InitProfileResponse -> ProfileCreateResponse
The function is now split into two functions: - librariesGetAllInstalled that requires a librariesmanager.Explorer. - libraryResolveDependencies that requires only a librariesindex.Index and do not require anymore a librariesmanager.Explorer.
baebee1 to
ce55e4b
Compare
| } | ||
|
|
||
| // DuplicateProfileError is returned when the profile is a duplicate of an already existing one | ||
| type DuplicateProfileError struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: Why not call it ProfileAlreadyExitsError, and clients know exactly what it means.
| } | ||
|
|
||
| // MissingProfileError is returned when the Profile is mandatory and not specified | ||
| type MissingProfileError struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProfileNotFoundError?
|
|
||
| // GRPCStatus converts the error into a *status.Status | ||
| func (e *MissingProfileError) GRPCStatus() *status.Status { | ||
| return status.New(codes.InvalidArgument, e.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to use NOT_FOUND.
| } | ||
|
|
||
| newProfile := &sketch.Profile{Name: req.GetProfileName(), FQBN: req.GetFqbn()} | ||
| // TODO: what to do with the PlatformIndexURL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, preferably, the CLI can tell if a resolved platform was detected by one of the additional board (aka. 3rd party) URLs. A missing feature (prop) on the platform object?
| `MyLib (1.0.0)`. | ||
| - `<INDEX_LIB_NAME> (<INDEX_LIB_VERSION>)` represents a library from the Arduino Libraries Index that is a direct | ||
| dependency of the sketch, for example, `MyLib (1.0.0)`. | ||
| - `dependency: <INDEX_LIB_NAME> (<INDEX_LIB_VERSION>)` represents a library from the Arduino Libraries Index that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I am not sure if I got this right. Can you please explain with this example? There is ALib@1 that depends on Base@1.0.0, and when the daemon adds ALib@1 to the profile, it will pin dependency: Base (1.0.0). Thank you!
| } | ||
|
|
||
| message ProfileCreateResponse { | ||
| // Absolute path to the project file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Absolute path to the project file. | |
| // Absolute path to the sketch profile file. |
?
| // The library to add to the profile. | ||
| ProfileLibraryReference library = 4; | ||
| // Set to true to add also all the dependencies of the library. | ||
| optional bool add_dependencies = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be much better to let clients opt out per library, rather than including all transitive dependencies by default.
like:
# rest
libraries:
- library: ArduinoIoTCloud (2.8.0)
excludeDeps: trueIf the CLI manually adds transitive dependencies to the sketch profile, it becomes a lock file, and not a dependency list file anymore. Users do not have to understand what the transitive dependencies are and how many there are. Most likely, compile will fail if clients do not pull the required libs, so users must have a good reason to opt out. Let them do that, and always include the dependencies required in the lib definition to add.
| // Set the profile as the default one if it's the only one | ||
| if req.DefaultProfile || len(sk.Project.Profiles) == 1 { | ||
| sk.Project.DefaultProfile = newProfile.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment does not reflect the code, but the code itself is strange; the client requests a profile creation, it explicitly sets DefaultProfile to false, but if it is the first profile, it will be set as the default anyway.
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)configuration.schema.jsonupdated if new parameters are added.What kind of change does this PR introduce?
This is an extract of #2917 containing only the gRPC part of the PR.
What is the current behavior?
Adds only the gRPC functions for managing libraries in profiles, no user-facing changes.
What is the new behavior?
Does this PR introduce a breaking change, and is titled accordingly?
No
Other information